-
Notifications
You must be signed in to change notification settings - Fork 791
[CI][UR] Bring up UR benchmarking results dashboard in intel.github.io #17475
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This patch improves numerous aspects on how the benchmarking results are visualized: - rewrites the way HTML charts are generated, using a library (Chart.js) that's both easier to use and more visually pleasing. The new HTML page also now decouples data from the HTML itself, leading to faster load times and the ability to fetch data from remote sources. - The markdown output now contains a failures section that lists all benchmarks that failed for a given run. This will be a helpful for developers during PR testing. - Benchmarks can now have description that's displayed on the page. - And many more minor improvements.
…to unify-benchmark-ci
This reverts commit f79bbbf.
... and add presets to more easily
…to unify-benchmark-ci
…to unify-benchmark-ci
On PRs based on main, the scripts location is "old" and not accesible. Pick location based on the dir existance. Step 'gather info' is in a 'weird' location, so solve it with 2 tries to execute the script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
jobs: | ||
build_sycl: | ||
name: Build SYCL from PR | ||
if: inputs.commit_hash != '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
techincally passing empty commit_hash
is allowed (required == false), so maybe it's not the best way of checking if this is a PR run or nigthly...?
// FYI, trigger type can be used to establish if nightly or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! think I resolved this via the latest commit: I've set default values on commit_hash to always be a string
// trigger type can be used to determine if a job was scheduled, but I've chosen to use commit_hash here incase the user themselves wants to use the nightly build of sycl manually
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm tbh, I'm not sure if setting default: ''
will solve the: != ''
, buuut If this will be an issue it can be updated later on
jobs: | ||
build_sycl: | ||
name: Build SYCL from PR | ||
if: inputs.commit_hash != '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe I'm missing something, but can we easily read the hash commit from PR?
I know we used pr_no
and a tricky fetching to get the proper commit from PR (https://github.com/intel/llvm/blob/unify-benchmark-ci/.github/workflows/ur-benchmarks-reusable.yml#L79-L81)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sycl-linux-build.yml job in intel/llvm doesn't support compiling commits from other branches, thus we cannot access commits from PR branches as of now: I have chosen to not stir the nest here, but this will probably be a future change that involes changing sycl-linux-build.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm... so what should I pass here to check my PR? or is it intended as a post-merge check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Admittedly I dont think that'd be possible right now 😅
We will figure something out though, given that this PR is no longer urgent, I'll make a PR to change sycl-linux-build.yml so that we can actually accomplish this
…nchmark.yml" This reverts commit 21a0599.
Sorry for the commotion everyone: It was decided in a meeting that getting this dashboard up by the end of today is no longer urgent. Thus, I'm closing this PR as it no longer makes sense to push this 2 versions of the benchmarking workflow to intel/llvm. I'll be folding these changes back into #17229. |
There has been work in #17229 to unify the UR and SYCL benchmarking CIs. However, given recent priorities on getting the UR's nightly dashboard up for intel/llvm, this PR lands some of the commits in #17229 that are ready to be deployed already: